fix: improve wallet encryption robustness #6945
Conversation
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
WalkthroughThe changes add defensive guards around HD chain encryption state in wallet key management. In src/wallet/scriptpubkeyman.cpp, decryption/encryption calls are conditioned on the HD chain existing and being flagged crypted, avoiding operations on uninitialized or already-targeted state. In src/wallet/wallet.cpp, EncryptWallet now re-checks encryption state inside the wallet lock, validates the decryption key for legacy paths before key refresh, and treats Rewrite() failure as an error path. No public APIs or signatures were changed. Sequence DiagramsequenceDiagram
participant Caller
participant EncryptWallet
participant ScriptPubKeyMan
participant HDChain
Caller->>EncryptWallet: Initiate wallet encryption
EncryptWallet->>EncryptWallet: Lock & re-check already encrypted (NEW GUARD)
alt Already encrypted
EncryptWallet-->>Caller: Return false
else Not encrypted
EncryptWallet->>ScriptPubKeyMan: If legacy & HD enabled, CheckDecryptionKey(vMasterKey) (NEW CHECK)
alt Invalid key
ScriptPubKeyMan-->>EncryptWallet: Return false
EncryptWallet-->>Caller: Return false
else Valid key or not applicable
EncryptWallet->>ScriptPubKeyMan: TopUp (if used)
EncryptWallet->>EncryptWallet: Rewrite() (NEW SUCCESS CHECK)
alt Rewrite fails
EncryptWallet->>EncryptWallet: Log error
EncryptWallet-->>Caller: Return false
else Rewrite succeeds
EncryptWallet->>ScriptPubKeyMan: EncryptKeys / EncryptHDChain (guarded)
EncryptWallet-->>Caller: Return true
end
end
end
sequenceDiagram
participant Caller
participant ScriptPubKeyMan
participant HDChain as "HD Chain State"
Caller->>ScriptPubKeyMan: GetKey / DeriveNewChildKey / UpgradeKeyMetadata
ScriptPubKeyMan->>HDChain: Obtain current or temp HD chain
alt Chain is non-null AND crypted (NEW GUARD)
ScriptPubKeyMan->>HDChain: DecryptHDChain / GetDecryptedHDChain
HDChain-->>ScriptPubKeyMan: Decrypted chain
else Chain is null OR not crypted
ScriptPubKeyMan->>ScriptPubKeyMan: Skip decryption/encryption calls (NEW BRANCH)
end
alt EncryptHDChain
ScriptPubKeyMan->>HDChain: Check if already crypted (NEW GUARD)
alt Already crypted
ScriptPubKeyMan-->>Caller: Return false
else Not crypted
ScriptPubKeyMan->>HDChain: Perform encryption
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/wallet/wallet.cpp (1)
792-795: Rewrite failure only logs; consider surfacing to caller or setting a persistent warning flagIf Rewrite() fails, we log but proceed. Consider returning false or setting a wallet flag that surfaces in RPC/UI so operators can act, and covering this path in tests.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/wallet/scriptpubkeyman.cpp(6 hunks)src/wallet/wallet.cpp(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.{cpp,h,cc,cxx,hpp}: Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1
Dash uses unordered_lru_cache for efficient caching with LRU eviction
Files:
src/wallet/wallet.cppsrc/wallet/scriptpubkeyman.cpp
🧠 Learnings (4)
📓 Common learnings
Learnt from: kwvg
Repo: dashpay/dash PR: 6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.
Learnt from: kwvg
Repo: dashpay/dash PR: 6761
File: src/chainlock/signing.cpp:247-250
Timestamp: 2025-07-29T14:32:48.369Z
Learning: In PR #6761, kwvg acknowledged a null pointer check issue in ChainLockSigner::Cleanup() method but deferred it to follow-up, consistent with the pattern of avoiding scope creep in refactoring PRs.
Learnt from: knst
Repo: dashpay/dash PR: 6805
File: src/wallet/rpc/wallet.cpp:357-357
Timestamp: 2025-08-08T07:01:47.332Z
Learning: In src/wallet/rpc/wallet.cpp, the upgradetohd RPC now returns a UniValue string message (RPCResult::Type::STR) instead of a boolean, including guidance about mnemonic backup and null-character passphrase handling; functional tests have been updated to assert returned strings in several cases.
Learnt from: kwvg
Repo: dashpay/dash PR: 6529
File: src/wallet/rpcwallet.cpp:3002-3003
Timestamp: 2025-02-14T15:19:17.218Z
Learning: The `GetWallet()` function calls in `src/wallet/rpcwallet.cpp` are properly validated with null checks that throw appropriate RPC errors, making additional validation unnecessary.
📚 Learning: 2025-08-08T07:01:47.332Z
Learnt from: knst
Repo: dashpay/dash PR: 6805
File: src/wallet/rpc/wallet.cpp:357-357
Timestamp: 2025-08-08T07:01:47.332Z
Learning: In src/wallet/rpc/wallet.cpp, the upgradetohd RPC now returns a UniValue string message (RPCResult::Type::STR) instead of a boolean, including guidance about mnemonic backup and null-character passphrase handling; functional tests have been updated to assert returned strings in several cases.
Applied to files:
src/wallet/wallet.cppsrc/wallet/scriptpubkeyman.cpp
📚 Learning: 2025-02-14T15:19:17.218Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6529
File: src/wallet/rpcwallet.cpp:3002-3003
Timestamp: 2025-02-14T15:19:17.218Z
Learning: The `GetWallet()` function calls in `src/wallet/rpcwallet.cpp` are properly validated with null checks that throw appropriate RPC errors, making additional validation unnecessary.
Applied to files:
src/wallet/wallet.cppsrc/wallet/scriptpubkeyman.cpp
📚 Learning: 2025-07-29T14:32:48.369Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6761
File: src/chainlock/signing.cpp:247-250
Timestamp: 2025-07-29T14:32:48.369Z
Learning: In PR #6761, kwvg acknowledged a null pointer check issue in ChainLockSigner::Cleanup() method but deferred it to follow-up, consistent with the pattern of avoiding scope creep in refactoring PRs.
Applied to files:
src/wallet/scriptpubkeyman.cpp
🧬 Code graph analysis (1)
src/wallet/wallet.cpp (3)
src/wallet/hdchain.cpp (2)
IsCrypted(37-41)IsCrypted(37-37)src/wallet/wallet.h (1)
IsCrypted(463-502)src/wallet/interfaces.cpp (1)
spk_man(214-221)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: mac-build / Build source
- GitHub Check: win64-build / Build source
- GitHub Check: linux64_nowallet-build / Build source
- GitHub Check: arm-linux-build / Build source
- GitHub Check: linux64_tsan-build / Build source
- GitHub Check: linux64_ubsan-build / Build source
- GitHub Check: linux64_sqlite-build / Build source
- GitHub Check: linux64_fuzz-build / Build source
- GitHub Check: linux64-build / Build source
- GitHub Check: Lint / Run linters
🔇 Additional comments (7)
src/wallet/wallet.cpp (1)
723-728: Good TOCTOU guard on encryption stateRe-checking IsCrypted() under cs_wallet closes the race window. LGTM.
src/wallet/scriptpubkeyman.cpp (6)
250-254: Prevent double-encrypting HD chainEarly-return when hdChainCurrent is already crypted is correct and matches the PR goal.
348-354: Only decrypt when cryptedThe additional guard avoids unnecessary DecryptHDChain calls.
485-491: GetDecryptedHDChain: skip decryption unless neededConditional decrypt keeps behavior tight and avoids redundant work.
506-508: EncryptHDChain: idempotence guardReturning false if already crypted prevents accidental re-encryption attempts.
1177-1183: GetKey: decrypt HD chain only when cryptedThe guard reduces unnecessary decryption attempts and aligns with robustness goals.
1294-1300: DeriveNewChildKey: conditional decryptionCorrectly avoids decrypting unencrypted chains.
Add defensive checks to prevent double-encryption of HD chains and improve wallet encryption robustness: - Check IsCrypted() status to prevent re-encryption attempts - Add TOCTOU protection by re-checking IsCrypted() under lock - Validate decryption keys before TopUp operations - Only decrypt HD chains when actually encrypted - Remove irrelevant HasEncryptionKeys() checks (keys passed as parameters) - Log warning when db rewrite fails after encryption
src/wallet/scriptpubkeyman.cpp
Outdated
| })) { | ||
| throw std::runtime_error(std::string(__func__) + ": DecryptHDChain failed"); | ||
|
|
||
| if (!hdChainCurrent.IsNull() && hdChainCurrent.IsCrypted()) { |
There was a problem hiding this comment.
I think condition here should be changed.
Shouldn't it be something like:
if (!hdChainCurrent.IsNull()) {
if (hdChainCurrent.IsCrypted() != m_storage.HasEncryptionKeys()) throw " no keys for encrypted wallet!";
...
}
Otherwise I don't see a sense in it
`GetHDChain` should catch this already
3 participants
Issue being fixed or feature implemented
Add defensive checks to prevent double-encryption of HD chains and improve wallet encryption robustness:
What was done?
How Has This Been Tested?
run tests
Breaking Changes
n/a
Checklist: